Skip to content

Update django form mutation to more granularly handle input/output fields #934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jtratner
Copy link

Example of how I'd like to change DjangoFormMutation as per #933. I think this should be backwards compatible with the existing one. Check out docstring on DjangoFormMutation for an overview of what the changes (ought to!) do :)

I also tried to add some docs about class behavior.

This still needs a test case and probably to be merged into the public graphene django docs but I figured this'd be good basis for discussion :)

Ultimately this is motivated by me having desire to be able to have a graphql mutation that just returns clientMutationId and a single object even tho I'm not using a DjangoModelFormMutation.

@jkimbo
Copy link
Member

jkimbo commented Apr 16, 2020

Thanks @jtratner ! I get the use case for this but I'm just wondering if this is the best API for it because it seems quite verbose. Could we simply it by just having 2 options: input_fields and output_fields where each is a list of fields or the string __all__ to select all fields? Is there much benefit to being able to specify only and exclude?

Related: for DjangoObjectType's we are planning on removing the only_fields and exclude_fields attributes in favour of only and exclude attributes: #691 We should probably maintain that naming convention elsewhere in the library as well.

@jtratner
Copy link
Author

@jkimbo - totally agree that it’s wordy 😂😅 I was trying to follow the existing convention.

Honestly if I were designing this class from the start I prob would have users explicitly specify output fields for the mutation. I’m not clear on a use case where you’d want to post the fields from cleaned_data back. Then you’d only need only and include to manage the input fields.

If you wanted to do that string, I’d suggest defining a constant

import graphene_django

...
Meta:
    input = graphene_django.ALL_FIELDS
    output = ()

@jkimbo
Copy link
Member

jkimbo commented Apr 16, 2020

I think that api makes more sense @jtratner so let's move towards that implementation. We can first start by preferring input_fields (or maybe just input) and output_fields options and emitting deprecation warnings if only_fields or exclude_fields are defined. Then in a later release we can drop only_fields and exclude_fields completely and also raise exceptions if input_fields or output_fields are not defined. I would follow a similar logic to #691

Don't have a strong opinion on defining a constant verses just writing out __all__. They can be both be valid. (The __all__ came from DjangoRestFramework)

@jtratner
Copy link
Author

jtratner commented Apr 16, 2020

What about getting rid of the automatic output fields in a backwards compatible way:

  1. only_fields and exclude_fields keep the existing behavior (and warns about deprecation)
  2. No fields specified keeps existing behavior (deprecation warning about required to specify)
  3. Specifying fields or exclude will be required in future version (they only modify input fields). If specified in current version, it means that only output field is clientMutationId. exclude=graphene_django.ALL_FIELDS means no input fields at all.

This would make the API much simpler

@jkimbo
Copy link
Member

jkimbo commented Apr 16, 2020

Specifying fields or exclude will be required in future version (they only modify input fields). If specified in current version, it means that only output field is clientMutationId. exclude=graphene_django.ALL_FIELDS means no input fields at all.

DjangoFormMutations has an errors field defined on the output type which should stay there. Also I think we should just drop the clientMutationId field completely because it's no longer necessary for relay or any other frontend client.

I think I like the idea of defaulting to no output fields which means you have to be very explicit about what outputs you want. Unfortunately I don't have many usecases for DjangoFormMutation so I'm not that familiar with what people use it form. What kind of things are you using it form @jtratner ?

@jtratner
Copy link
Author

Our app (for good or ill) has a number of forms that are pretty-close-but-not-quite ModelForms. (So they essentially construct objects, just the input is different than underlying DB representation).

This class is handy cuz it lets us reuse validation logic from our existing HTML front end (old school without JS)

That said, looking at Django Rest Framework, I wonder if write_only and read_only would be better names (with anything not specified assumed to be for both). This would match the serializer definitions...

(Apologies for the array of ideas - just hoping to get to a meaningful API)

@jtratner
Copy link
Author

(See #933 for a simplifies version of our use case)

Set up forms

Update docs

Nearly working tests
@jtratner jtratner force-pushed the example-changes-to-django-form-mutation branch from a2da4ab to 4246dab Compare May 15, 2020 05:51

result = schema.execute(
""" mutation MyMutation {
myMutation(input: { text: "VALID_INPUT" }) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkimbo - I am super confused why this test errors with text not a valid keyword argument when the assertion statements above are true. Any thoughts?

>       assert result.errors is None
E       assert [GraphQLLocatedError("'text' is an invalid keyword argument for MyMutation")] is None
E        +  where [GraphQLLocatedError("'text' is an invalid keyword argument for MyMutation")] = <graphql.execution.base.ExecutionResult object at 0x103d25af8>.errors```

@stale
Copy link

stale bot commented Aug 27, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Aug 27, 2020
@ulgens ulgens removed the wontfix label Aug 28, 2020
@zbyte64 zbyte64 changed the base branch from master to v2 December 31, 2020 06:37
@jtratner
Copy link
Author

jtratner commented Apr 7, 2021

@jkimbo - circling back to this after a while :) Has your API break completed now? (I think that was a blocker in the past). If so, is there a set way of naming that you'd like me to use? If not, I can try to get back in the zone for this PR and finish it up.

@firaskafri firaskafri closed this Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants